Conversation
growable list was not actually faster
|
One comment: given your comment on #501 do we want to say that this PR fixes #501? Or just that it helps? |
To fix #501, or do more towards it, can we increase the page size (by exposing the arg if it isn't already, and setting its default) to 500, rather than the default from the server of 20? https://docs.posit.co/connect/api/#get-/v1/instrumentation/content/visits |
I think technically the way 501 is written, this closes it. We can improve performance of other functions that will help the ultimate use case (
@joshyam-k discovered that when this function is called with Line 725 in 1bfa7ea I do think that exposing the page size is a good idea though and will open an issue for that as well. edit: added #506 |
Looks like that's setting limit, not page size? Maybe I'm missing something but when I traced this through before, I didn't see where a |
Yeah it is very confusing. For the API, |
|
Oh yeah wow that is confusing! Thanks for straightening me out. |
3 participants
Intent
Fixes #501
Approach
We were appending results iteratively with
c()and, while not the slowest part of this code, it was improved by growing the list page by page and then combining once at the end withdo.call(). Unfortunately we cannot pre-allocate the list because the API does not return the total number of pages, we have to go through them one by one until we reach the end.For a
get_usage_static()call returning 175k results this shaved off about half a second (~11.5 seconds to just under 11). Not a huge improvement but still worth making since the code is no more complex.While I was in there I also noticed some incorrect documentation and unnecessary tidy evaluation that I've also removed.
Checklist
NEWS.md(referencing the connected issue if necessary)?devtools::document()?